Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IP address preference support for TCP connection #1015

Merged
merged 29 commits into from
May 17, 2021

Conversation

karinazhou
Copy link
Member

@karinazhou karinazhou commented Mar 31, 2021

A new connection property IPAddressPreference is added to specify the IP preference by the client application when doing TCP connection. There are three valid types:

  • IPv4First
    This is the default preference. The driver will traverse IPv4 addresses first. If none of them can be connected successfully, it continues to try IPv6 addresses if there are any.

  • IPv6First
    The driver will traverse IPv6 addresses first. If none of them can be connected successfully, it continues to try IPv4 addresses if there are any.

  • UsePlatformDefault
    The driver will traverse all the IP addresses in their initial orders from DNS resolution.

To keep the consistency between managed SNI and native SNI, the connection logic is changed in SNITcpHandle:Connect(). Instead of keeping the last IPv4 and IPv6 addresses, all the valid IP addresses returned from DNS resolution will be tested in a row. There will be a separate PR containing this change in parallel.

NOTE: This PR requires the change in native SNI so it requires a new version of Microsoft.Data.SqlClient.SNI to be released first.

cc @johnnypham @cheenamalhotra @David-Engel @saurabh500

@Wraith2
Copy link
Contributor

Wraith2 commented Apr 2, 2021

I had a talk to someone with some networking experience and they pointed out that many applications will use addresses in the order returned from dns to connect to resources and so the first ip address in a list is likely to be the most connected route. This means that seemingly arbitrary choice of the last ip of each type may have been a deliberate attempt to choose the least used route. If this is true then that should have been called out in the code with a comment.

I think trying each address returned in the order that they are returned will be more compatible with dns based load balancing schemes. When you look at parallelizing the connections you may want to keep track of the original ordering and when two connections succeed choose the first using the original ordering.

@cheenamalhotra cheenamalhotra added the 🆕 Public API Use this label for new API additions to the driver. label Apr 9, 2021
@cheenamalhotra cheenamalhotra added this to the 3.0.0-preview3 milestone Apr 9, 2021
johnnypham and others added 4 commits April 23, 2021 12:47
# Conflicts:
#	src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj
BUILDGUIDE.md Outdated Show resolved Hide resolved
@DavoudEshtehari DavoudEshtehari changed the title WIP | Add IP address preference support for TCP connection Add IP address preference support for TCP connection May 10, 2021
Davoud Eshtehari and others added 2 commits May 11, 2021 10:36
Co-authored-by: Javad <v-jarahn@microsoft.com>
Co-authored-by: Cheena Malhotra <v-chmalh@microsoft.com>
+ improvement
Co-authored-by: Cheena Malhotra <v-chmalh@microsoft.com>
Comment on lines 453 to 454
string sValue = (value as string);
if (sValue is not null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be if (value is string sValue) ? the is operator does the type and null checks so it's known to be a non-null string inside the if block if the condition returns true.

+ modify CancelAsyncConnections test
DavoudEshtehari and others added 2 commits May 14, 2021 12:31
…eference.xml

Co-authored-by: Cheena Malhotra <v-chmalh@microsoft.com>
+ Disable TNIR in connection string
@cheenamalhotra cheenamalhotra merged commit 561b535 into dotnet:main May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 Public API Use this label for new API additions to the driver.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants